Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

record reflection of converted types #808

Merged
merged 1 commit into from
Nov 18, 2023
Merged

record reflection of converted types #808

merged 1 commit into from
Nov 18, 2023

Conversation

lu4p
Copy link
Member

@lu4p lu4p commented Nov 17, 2023

fixes #807, fixes #782, fixes #763, fixes #785

@lu4p
Copy link
Member Author

lu4p commented Nov 17, 2023

The reverse test fails, because it expects a failure for the exact case that is fixed now.
I don't think I can construct a case that fails with garble, but not with go anymore.
I'm going to remove it.

Copy link
Member

@mvdan mvdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting - I had a partial fix locally for these issues, and I had also gone with the same approach of propagated instructions and ChangeType :) not exactly the same as yours, but similar.

testdata/script/reverse.txtar Show resolved Hide resolved
Copy link
Member

@mvdan mvdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job! This SSA code is always a bit tricky to properly understand and review, but I don't see anything obviously wrong with it, and I can't argue with the number of bugs fixed :)

I assume that the added test cases already cover all the edge cases from the four bugs you're fixing. If that's not the case for an of them, please send a follow-up PR adding more test cases, verifying that they fail if you revert the fix. Good regression tests are absolutely crucial to make sure that our SSA code works like it's meant to.

testdata/script/reverse.txtar Show resolved Hide resolved
@mvdan mvdan merged commit bec8043 into master Nov 18, 2023
5 checks passed
@mvdan mvdan deleted the fixSSH branch November 18, 2023 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants